[#4236] feat(core): Rework of post hook for dispatcher #4429
Conversation
jerqi
changed the title
| for (CatalogCreatePostHook hook : createPostHooks) { | ||
| hook.execute(ident); | ||
| } |
There was a problem hiding this comment.
I think we can directly execute the here, didn't need to call hook.execute().
ownerManager.setOwner(
ident.name(),
NameIdentifierUtil.toMetadataObject(ident, Entity.EntityType.METALAKE),
PrincipalUtils.getCurrentUserName(),
Owner.Type.USER);
There was a problem hiding this comment.
I feel that we should extract the abstraction of the hook. It will be convenient to extend more post hooks.
There was a problem hiding this comment.
I think we didn't need this class.
jerqi
force-pushed
the
ISSUE-4236
branch
6 times, most recently
from
5f296b0 to
4454e2b
Compare
jerqi
force-pushed
the
ISSUE-4236
branch
2 times, most recently
from
79561de to
53aaa1a
Compare
jerqi
force-pushed
the
ISSUE-4236
branch
4 times, most recently
from
3949efc to
d967218
Compare
| @@ -33,6 +33,17 @@ public class SecurableObjects { | |||
|
|
|||
| private static final Splitter DOT_SPLITTER = Splitter.on('.'); | |||
|
|
|||
| /** | |||
| * Create the metalake {@link SecurableObject} with the given metalake name. | |||
There was a problem hiding this comment.
With the given metalake name and privileges?
| import org.apache.gravitino.utils.PrincipalUtils; | ||
|
|
||
| /** | ||
| * {@code AccessControlHookDispatcher} is a decorator for {@link AccessControlDispatcher} that not |
There was a problem hiding this comment.
What the meaning of decorator in this context?
There was a problem hiding this comment.
You can refer to decorator design pattern.
| * only delegates access control operations to the underlying access control dispatcher but also | ||
| * executes some hook operations before or after the underlying operations. | ||
| */ | ||
| public class AccessControlHookDispatcher implements AccessControlDispatcher { |
There was a problem hiding this comment.
I can't get why do we name it Hook here?
There was a problem hiding this comment.
This class provide the hook operations for every dispatcher.
| this.catalogManager = new CatalogManager(config, entityStore, idGenerator); | ||
| CatalogHookDispatcher catalogHookDispatcher = new CatalogHookDispatcher(catalogManager); | ||
| CatalogNormalizeDispatcher catalogNormalizeDispatcher = | ||
| new CatalogNormalizeDispatcher(installDispatcherHooks((CatalogDispatcher) catalogManager)); | ||
| new CatalogNormalizeDispatcher(catalogHookDispatcher); |
There was a problem hiding this comment.
It seems that we have wrapped the dispatcher too many times.
There was a problem hiding this comment.
Known issues. But we don't have better solution.
There was a problem hiding this comment.
Maybe we can put normalizeDispatcher to HookDispatcher in the future.
There was a problem hiding this comment.
I also feel the current Dispatcher mechanism inelegant.
I create a issue track it
xunliu
changed the title
|
I don't like the current package name "hook", it doesn't have any meaning. Besides, the changes are related to authorization, why don't you move to authorization? Anyway, please refactor this code. |
Co-authored-by: Rory <[email protected]>
Normalizing the names may be a pre-hook. I plan to add some authorization unrelated hooks to this class to reduce the number of dispatcher. |
What changes were proposed in this pull request?
Rework of post hook for dispatcher .
Why are the changes needed?
Fix: #4236
Does this PR introduce any user-facing change?
NO.
How was this patch tested?
Add new ITs.